Skip to content

Address review follow-ups on the async enclave attestation gate - #4621

Open
cheenamalhotra wants to merge 2 commits into
dev/automation/async-enclave-providersfrom
dev/automation/async-enclave-providers-followups
Open

Address review follow-ups on the async enclave attestation gate#4621
cheenamalhotra wants to merge 2 commits into
dev/automation/async-enclave-providersfrom
dev/automation/async-enclave-providers-followups

Conversation

@cheenamalhotra

Copy link
Copy Markdown
Member

Stacked on #4541. Addresses the remaining open review comments there.

Changes

Comment Change
Copilot: unused using Removed using Microsoft.Data.Common; from AzureAttestationBasedEnclaveProvider.cs and VirtualSecureModeEnclaveProviderBase.cs. They became unused when the redundant overrides were deleted.
@mdaigle: Task.Delay units Both async retry loops now use TimeSpan.FromSeconds(...) instead of x * 1000.
@priyankatiwari08 (3): wrong comment ThreadRetryCache stores thread IDs for the sync path, not the attestation url and nonce. Comment corrected.
@priyankatiwari08 (1): sync lock timeout never restored Confirmed intentional, now documented. The async gate uses LockTimeoutMaxInMilliseconds directly and never reads or writes the sync path's adaptive lockTimeoutInMilliseconds. The decoupling is symmetric: async callers cannot degrade that value for sync callers, and sync contention cannot collapse the async timeout to zero.
@priyankatiwari08 (2): missing tests Added the two suggested tests (below).

Tests

  • CreateEnclaveSessionAsync_WhenGateWaitTimesOut_AttestsAnyway - a caller that cannot take the gate within the timeout attests on its own instead of failing or deadlocking, and does not release a gate it never held.
  • GetEnclaveSessionAsync_AfterInvalidation_ReattestsAndReturnsNewSession - after invalidation the next async caller re-attests and gets a new session, which is then cached.

To support the first test, AsyncAttestationGateTimeoutInMilliseconds is a protected virtual hook on EnclaveProviderBase so a test provider can shorten the wait. Production providers use the default 15s.

The fake provider also tracks a high-water mark of concurrent attestations. Collapsing and fallthrough are now asserted directly (MaxConcurrentAttestations of 1 vs 2) instead of inferred from timing, and the fallthrough test parks the gate holder on a TaskCompletionSource the test controls rather than sleeping. Verified both tests fail if the fallthrough is removed.

Not included

@mdaigle's larger suggestion to converge the sync and async paths onto one primitive was marked "Not for this PR". It needs the sync path reshaped first (acquire/release within CreateEnclaveSession, post-gate cache re-check, dropping the cross-call handoff and the timeout mutation) and a change to EnclaveSessionCache.CreateSession to return an existing entry rather than overwrite. Worth a follow-up issue.

Checklist

  • Tests added or updated
  • Public API changes documented (no public API change)
  • Verified against customer repro (n/a)
  • Ensure no breaking changes introduced

- Drop the now-unused Microsoft.Data.Common usings from the Azure and VSM
  providers, left behind when their redundant overrides were removed.
- Use the TimeSpan overload of Task.Delay in both async retry loops.
- Correct the ThreadRetryCache comment: it records thread IDs for the sync
  path, not the attestation url and nonce.
- Document that the async gate deliberately does not share the sync path's
  adaptive lock timeout, so neither path can degrade the other.
- Make the async gate timeout overridable so tests can drive the timeout
  fallthrough without waiting out the production timeout.
- Add tests for the gate timeout fallthrough and for async re-attestation
  after a session is invalidated, plus a concurrency high-water mark on the
  fake so collapsing and fallthrough are asserted directly rather than by
  timing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 148862f6-f66a-4a89-8436-ec4a008bbea3

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Addresses review follow-ups for the asynchronous enclave attestation gate.

Changes:

  • Uses explicit TimeSpan retry delays and removes unused imports.
  • Documents sync/async gate separation and corrects retry-cache documentation.
  • Adds timeout, concurrency, and invalidation tests.
File summaries
File Description
SqlColumnEncryptionEnclaveProviderAsyncShould.cs Adds gate and invalidation tests.
VirtualSecureModeEnclaveProviderBase.cs Removes an unused import.
VirtualSecureModeEnclaveProvider.cs Clarifies retry-delay units.
EnclaveProviderBase.cs Documents and exposes the testable gate timeout.
AzureAttestationBasedEnclaveProvider.cs Removes an import and clarifies retry-delay units.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +368 to +379
Task<SqlEnclaveSession> blocked = Task.Run(() => AttestAsync(provider, blockedParameters));

// Reaching two attestations while the first is still parked is only possible if the second
// caller gave up on the gate. Without the fallthrough this wait times out.
await provider.WaitForAttestationCountAsync(2);
Assert.Equal(2, provider.MaxConcurrentAttestations);

hold.SetResult(true);
provider.HoldAttestation = null;

SqlEnclaveSession heldSession = await holder;
SqlEnclaveSession blockedSession = await blocked;
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 1, 2026 19:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AlwaysEncrypted/SqlColumnEncryptionEnclaveProviderAsyncShould.cs:788

  • Add an XML summary for this new test helper. Test helper methods in this repository are required to document their behavior and side effects; this one updates both the active-attestation count and its high-water mark.

This issue also appears on line 809 of the same file.

            private void EnterAttestation()

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/AlwaysEncrypted/SqlColumnEncryptionEnclaveProviderAsyncShould.cs:809

  • Add the required XML summary for this new test helper as well, so its counter side effect is documented consistently with the other helpers in this file.
            private void ExitAttestation() => Interlocked.Decrement(ref _concurrentAttestations);
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@paulmedynski paulmedynski assigned mdaigle and unassigned paulmedynski Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

5 participants